New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TLS config: Enable selection of min TLS version #375
Conversation
go1.18 changes the default minimum TLS version to 1.2. Let's make the default minimum version configurable, while following go default. The allowed values (TLS10, ..) come from the exporter-toolkit: https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md TLSVersion is exported so the exporter toolkit can reuse them later. Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks ok. I put a couple of comments / notes.
config/http_config.go
Outdated
@@ -714,6 +750,8 @@ type TLSConfig struct { | |||
ServerName string `yaml:"server_name,omitempty" json:"server_name,omitempty"` | |||
// Disable target certificate validation. | |||
InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"` | |||
// Minimum TLS version. | |||
MinVersion TLSVersion `yaml:"min_version,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is missing here the json tag
MinVersion TLSVersion `yaml:"min_version,omitempty"` | |
MinVersion TLSVersion `yaml:"min_version,omitempty" json:"min_version,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if MinVersion
is not mandatory, don't you want to provide a default value ?
You could do it by overriding the Unmarshal's method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I want to use go default. I think we should let them define the default while providing our users a way to divert if really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm ok. And I guess in Prometheus you will define that the default / Minimum version is the previous one used, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the plan is to mark it as a CHANGE in the changelog.
I think out users will benefit from defaults that go runtime provides. We should probably not make the call.
Possibly it can break some users, but they have the option to opt-out this change by explicitly setting tls1.0 as supported version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can agree on this assumption and I agree it's better to use the default tls version supported by Go.
I'm just wondering how much it will impact the users.
Let's see if someone answers to your email.
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
ef41c00
to
69a7baf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks !
go1.18 changes the default minimum TLS version to 1.2.
Let's make the default minimum version configurable, while following go
default.
The allowed values (TLS10, ..) come from the exporter-toolkit:
https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md
TLSVersion is exported so the exporter toolkit can reuse them later.
Signed-off-by: Julien Pivotto roidelapluie@o11y.eu